Build the whole compiler with dune #5820

cknitt · 2022-11-17T06:35:49Z

This PR simplifies the ReScript build setup and install scripts, improves the developer experience, reduces npm package size and removes repo bloat.

The existing build setup

relies on ninja and bspack for the compiler executables
relies on checking in build artifacts into the repo, causing repo bloat
needs the compiler to be built twice (once to build the stdlib and once more with the builtin_cmi_datasets/builtin_cmj_datasets from that stdlib build)

All of this is replaced with a simple setup based on the standard OCaml build tool, dune. Building all compiler binaries is now as easy as checking out the sources and running

opam install . --deps-only
dune build

The cmi/cmj cache is not compiled into the compiler binaries anymore, but kept in external binary files loaded by the compiler on demand. This also helps in reducing package size. My testing showed no performance regressions.

Support for building the compiler from source in postinstall is removed. As described in #5694, this shall be replaced by providing pre-built binaries for more platforms and, to support exotic platforms, by allowing the user to specify the binaries to be used via an environment variable on npm install. This also obviates the need for vendoring the ocaml and ninja sources.

Some positive effects of these changes include:

No need for snapshotting anymore, simplifying development/contribution, reducing repo size and noise in PRs.
The size of the npm package, already down from 47.1 MB in 10.1 to 33.9 MB in 11.0 (master), is further reduced to 25.9 MB.
Removes >690,000 LoCs of build artifacts and vendored code.
The compiler build with dune works on Windows. The previous "ninja config"/"ninja build" workflow didn't, because the command line grew too large. (This problem still exists for the stdlib build though which still relies on the ninja setup.)
It facilitates future improvements to the build setup, like adding binaries for more platform or building statically linked executables for Linux. I already tried the latter with very promising results.

Follow-up work for future PRs, as this one is already large enough:

Playground compiler: Jsoo compilation for the playground is already integrated in the dune build setup, but it is most likely broken/further work needed. I would look at this together with @ryyppy separately.
Further scripts cleanup and implementation of the mechanism described in Do not build compiler from source in postinstall #5694 for "bringing your own binaries".

cristianoc

This is an exciting development.
Bonus points for the reduced package size.

bobzhang · 2022-12-04T12:57:06Z

I am always paranoid about any performance slow dow -- it seems now it will do lots of IO when consulting standard libraries while it was embedded and self contained -- but I am fine to make some trade offs in exchange for easier contributions, thanks for spending time working on this!

Is this finished? Does test still depend on ninja?
Would you make js_of_ocaml-compiler optional?

cknitt · 2022-12-04T13:24:27Z

I am always paranoid about any performance slow dow -- it seems now it will do lots of IO when consulting standard libraries while it was embedded and self contained -- but I am fine to make some trade offs in exchange for easier contributions, thanks for spending time working on this!

Previously, the cmi / cmj cache data was loaded once as part of the executable, now it is loaded once from an external cache file, but the executable is smaller in exchange.

I was also worried a bit about possible performance implications. When testing with one of our projects which takes about 10s to build on my macOS ARM machine, I didn't really see any difference in build performance beyond statistical fluctuation.

Is this finished? Does test still depend on ninja?

Yes, this is finished except for what I listed under "Follow-up work" to be done in separate PRs.
Only the executables are now built with dune, the standard library build and the tests are still using ninja.

Would you make js_of_ocaml-compiler optional?

The jsoo build is currently commented out anyway in jscomp/jsoo/dune.
As stated under "Follow-up work", I'll still need to adapt the playground build workflow. I'll keep in mind to keep the jsoo build optional.

bobzhang · 2022-12-04T13:29:50Z

When testing with one of our projects which takes about 10s to build on my macOS ARM machine, I didn't really see any difference in build performance beyond statistical fluctuation.

The slow down may mostly come from the latency when the user just build one file (in IDE + slow file system).

cknitt · 2022-12-04T13:59:47Z

I would propose to merge this as is, and in case any performance issues are uncovered during the 11.x development cycle, we can revisit the cmi/cmj caching strategy. If needed, we can always revert to building the caches into the compiler, we would then need to modify the build/CI scripts to rebuild the compiler after having extracted the cache files.

bobzhang

You mentioned that test directory work, how does it work?
From what I read, the lib directory and test targets still depend on ninja, this workflow
still relies on ninja ?

bobzhang · 2022-12-05T01:38:02Z

Makefile


+format:
+	dune build @fmt --auto-promote
+


do you want to promote files from ml directory which came from upstream?

Currently, while there is a global .ocamlformat file, ocamlformat is disabled for the whole jscomp dir via jscomp/.ocamlformat. We can enable it selectively for the different jscomp subdirs in subsequent PRs.

bobzhang · 2022-12-05T01:41:42Z

jscomp/core/dune

+; There exist cyclic dependencies between core and frontend, therefore include frontend files here:
+
+(copy_files# ../frontend/*.{ml,mli})
+


what are the cyclic dependencies here?

core:

File "jscomp/core/lam_primitive.mli", line 44, characters 18-42: Error: Unbound module External_arg_spec File "jscomp/core/lam_compile_const.mli", line 29, characters 24-45: Error: Unbound module External_arg_spec File "jscomp/core/lam_compile_external_obj.mli", line 34, characters 2-30: Error: Unbound module External_arg_spec File "jscomp/core/lam_constant_convert.ml", line 32, characters 22-62: Error: Unbound module Ast_utf8_string_interp

frontend:

File "jscomp/frontend/ast_payload.mli", line 43, characters 7-27: Error: Unbound module Js_raw_info File "jscomp/frontend/ast_utf8_string_interp.mli", line 61, characters 45-52: Error: Unbound module J File "jscomp/frontend/classify_function.mli", line 25, characters 52-67: Error: Unbound module Js_raw_info

bobzhang · 2022-12-05T01:44:03Z

jscomp/napkin/dune

+(library
+ (name napkin)
+ (wrapped false)
+ (flags


since res_syntax is integrated now, we don't need do the copy here, we can just build it as is

The issue is that there are currently two different compiler libs in the repo:

res_syntax/compiler-libs-406: created by @cristianoc for the syntax repo in https://github.com/rescript-lang/rescript-compiler/pull/5820/files#diff-ae689c9d78337a2d2fdc22773dbd60e8c74cac71ac957d4d6a16a2a9d6fb7c93

jscomp/ml: the much more heavily modified one from the compiler repo itself

Currently, the syntax lib, cli and benchmarks (all in res_syntax) are built with 1., but for the compiler we want to build with 2. This is why the files need to be copied.

This behavior is not a change in the current PR, and the compiler libs duplication can be resolved in a future PR.

bobzhang · 2022-12-05T01:54:06Z

I am okay to move this forward after some details figured out. Just want to point out that your original benchmark is not exact since a single digit slow down would not be observable under a highly concurrent build.
The philosophy has been that whenever there is a conflict between performance and developer convenience, we always go in favor of the former, I hope we won't go too far from this.

cknitt · 2022-12-06T06:37:21Z

You mentioned that test directory work, how does it work? From what I read, the lib directory and test targets still depend on ninja, this workflow still relies on ninja ?

To clarify, only the following executables are built using dune:

% ls _build/install/default/bin/
bsb_helper		cmij			ounit_tests		rescript		syntax_tests
bsc			cmjdump			res_parser		syntax_benchmarks

The rest, i.e., the way the standard library and the files in jscomp/test are built, is unchanged, it still requires ninja. I do not know if it would be feasible to use dune for that, as dune does not support ReScript. It is not in the scope of this PR to change this.

@bobzhang Do you have any further questions? Can we merge? After merging, I will create issues for the items raised here (ocamlformat, cyclic dependency core <-> frontend, compiler libraries cleanup).

bobzhang · 2022-12-06T08:05:15Z

The rest, i.e., the way the standard library and the files in jscomp/test are built, is unchanged, it still requires ninja.

The benefit is unclear to me, previously we can build it without any dependencies (only C toolchain needed, now we have to depend on lots of things and gets slower), we have two build tools needed now.

If @cristianoc is happy with this , I am fine.

cristianoc · 2022-12-06T09:52:23Z

We're already using dune as a second-class citizen, after ninja succeeds, to support editor integration. This makes it first class and removes a bunch of weird situations during development.
I think ninja can be eliminated too, though it's another bunch of work and it seems safer to proceed in steps.
Happy to go ahead with this.

cknitt requested a review from cristianoc November 17, 2022 06:35

cknitt force-pushed the dune-project branch from dd6e7f3 to 93ceebe Compare November 17, 2022 06:46

cknitt marked this pull request as draft November 17, 2022 06:47

cknitt removed the request for review from cristianoc November 17, 2022 07:02

cknitt force-pushed the dune-project branch from 93ceebe to 47394b3 Compare November 19, 2022 08:24

cknitt changed the title ~~Remove dune-project from res_syntax and extend the one in root~~ Enable building the whole compiler with dune Nov 19, 2022

cknitt force-pushed the dune-project branch 8 times, most recently from b10eaa4 to e6b6fbf Compare November 20, 2022 07:18

cknitt changed the title ~~Enable building the whole compiler with dune~~ Build the whole compiler with dune Nov 20, 2022

cknitt force-pushed the dune-project branch 11 times, most recently from 77c9f7f to 66d50df Compare November 26, 2022 14:16

cknitt force-pushed the dune-project branch 4 times, most recently from a0224c1 to 23f1f59 Compare November 29, 2022 19:05

cknitt force-pushed the dune-project branch 7 times, most recently from f1d70f5 to e83bff2 Compare November 30, 2022 16:14

cknitt mentioned this pull request Dec 3, 2022

Do not build compiler from source in postinstall #5694

Closed

cknitt force-pushed the dune-project branch 3 times, most recently from f4ea04d to 3950dbe Compare December 3, 2022 09:16

Build the whole compiler with dune

7f4d09c

cknitt force-pushed the dune-project branch from 3950dbe to 7f4d09c Compare December 3, 2022 09:18

cknitt marked this pull request as ready for review December 3, 2022 09:43

cknitt requested review from cristianoc and bobzhang December 3, 2022 09:43

cristianoc reviewed Dec 3, 2022

View reviewed changes

cristianoc approved these changes Dec 4, 2022

View reviewed changes

bobzhang reviewed Dec 5, 2022

View reviewed changes

cristianoc merged commit 81a3dc6 into rescript-lang:master Dec 6, 2022

cknitt deleted the dune-project branch December 6, 2022 15:57

cknitt mentioned this pull request Dec 31, 2022

DRAFT - very first try at building rescript-compiler with dune #5687

Closed

		; There exist cyclic dependencies between core and frontend, therefore include frontend files here:

		(copy_files# ../frontend/*.{ml,mli})

Build the whole compiler with dune #5820

Build the whole compiler with dune #5820

Uh oh!

Conversation

cknitt commented Nov 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cristianoc left a comment

Choose a reason for hiding this comment

Uh oh!

bobzhang commented Dec 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cknitt commented Dec 4, 2022

Uh oh!

bobzhang commented Dec 4, 2022

Uh oh!

cknitt commented Dec 4, 2022

Uh oh!

bobzhang left a comment

Choose a reason for hiding this comment

Uh oh!

bobzhang Dec 5, 2022

Choose a reason for hiding this comment

Uh oh!

cknitt Dec 5, 2022

Choose a reason for hiding this comment

Uh oh!

bobzhang Dec 5, 2022

Choose a reason for hiding this comment

Uh oh!

cknitt Dec 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bobzhang Dec 5, 2022

Choose a reason for hiding this comment

Uh oh!

cknitt Dec 5, 2022

Choose a reason for hiding this comment

Uh oh!

bobzhang commented Dec 5, 2022

Uh oh!

cknitt commented Dec 6, 2022

Uh oh!

bobzhang commented Dec 6, 2022

Uh oh!

cristianoc commented Dec 6, 2022

Uh oh!

Uh oh!

cknitt commented Nov 17, 2022 •

edited

Loading

bobzhang commented Dec 4, 2022 •

edited

Loading

cknitt Dec 5, 2022 •

edited

Loading